Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Python package] Clearer code? #373

Merged
merged 12 commits into from
Jul 20, 2023
Merged

[Python package] Clearer code? #373

merged 12 commits into from
Jul 20, 2023

Conversation

lpascal-ledger
Copy link
Contributor

@lpascal-ledger lpascal-ledger commented May 15, 2023

Rationale

This PR does not simplify or restructure (much) the Python package code, but rather aims at highlighting module / class dependencies and showing off interfaces.

Some modules in the Speculos Python code has evolved fast lately, and without formal / explicit structure, it becomes hard to understand the code or the impact of changes.

I've started by typing the code, which is a huge help to explicit what's used where. In the meantime, mypy asserts my assumptions and helps revealing unexpected / strange things, circular dependencies and whatnot.

Second step was to extract interfaces from the code in a neutral place (most in the existing speculos.mcu.display module, some other in speculos.mcu.struct module, and a speculos.observer module) and try to change the code so that it relies more on the abstractions rather than concrete classes (that's basically a dependency inversion task, which will help simplifying intricate code).

I think the third step would be to then restructure the code so that the abstractions are stored in sensible place, and concrete classes are lighter and clearer. I'm not sure this step will be finished in this PR, as it may need more scrutiny and may take longer to merge than this one alone (and the bigger the PR, the more painful the rebasing).

Current changes are:

CI changes

  • Independent Python CI with flake8 (existed already), + mypy and bandit (this last is neutralized and will not redify the CI, although its output is interesting)
  • Changed the reusable workflow so that the Speculos image is built once and not for each device

Code

Typing

Adding as much typing hints as possible, slightly modifying the code to add clarity

Architecture

  • ObserverInterface and BroadcastInterface explicit an observer design pattern used through the AutomationServer and the EventsBroadcaster, and instantiated in the main function. The first one is tagged as legacy, so I suggest it should be cleaned for the next stable version. For now, the new interface helps keeping track of the current usage
  • Added a GraphicalLibrary interface for both Bagl and NBGL in order to ease their usage. Direct usage of either bagl or nbgl in the seproxyhal module should not occur, they should be hidden inside the screen (a Display implementation). It is not done currently, it may require more work, but at least GraphicalLibrary covers some FrameBuffer methods, so that there is no longer code like screen.nbgl.m.take_screenshot(), but instead screen.gl.take_screenshot(), with gl being a generic type (GraphicalLibrary, so either Bagl or NBGL).
    Unfortunately, as some SephTags are NBGL-specifics, the dispatcher in SeProxyHal.can_read uses assert isinstance(screen.gl, NBGL) which degrades a bit the abstraction.
  • The Display interface is used by TextScreen, Headless and Screen, however these implementation are not used consistently as the latter is shadowed by QtScreen which uses Screen through indirect composition. mypy clearly sees the issue, however the code is quite intricate here so not sure I'll flatten the dependencies here. => this has been answered in the last bullet of this list
  • This Display interface also implements a sort of unclear observer design pattern too through its notifiers property and methods. I've started to define an interface IODevice for these notified classes (often defined as klass), they seem to be related to instances stored into ServerArgs but I've not yet catched the meaning of this.
  • Back on the Display shenanigans, there is a very unclear way of managing the application graphic interfaces. There currently is 3 options: Text, Headless and Qt. The two firsts are quite similar, the last however, is very different and more complex. These classes are intertwined with the (now called) IODevice objects, and QtObject for the Qt one, with cross-dependencies all over the place (things like the notifiers and IODevice.can_read(klass)). First step I found there was to cut these classes in two: a DisplayNotifier, managing the event notifications, and a Display, used by the IODevices to interract with the screen. This way, the interfaces are clear: the main.py instantiates a DisplayNotifier and run its loop, and this DisplayNotifier can internally instantiate the correct Display and notify the IODevice. This is to be improved yet, but still sounds better than previously, where the QtScreen was really not on the same level as the others (Headless and TextScreen).

@lpascal-ledger lpascal-ledger changed the title OCR control Clearer code ? May 15, 2023
@lpascal-ledger lpascal-ledger changed the title Clearer code ? Clearer code? May 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

Patch coverage: 27.32% and project coverage change: +0.41 🎉

Comparison is base (91f9642) 48.93% compared to head (8b29543) 49.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
+ Coverage   48.93%   49.34%   +0.41%     
==========================================
  Files         113      115       +2     
  Lines       10854    11010     +156     
==========================================
+ Hits         5311     5433     +122     
- Misses       5543     5577      +34     
Impacted Files Coverage Δ
speculos/api/screenshot.py 50.00% <0.00%> (ø)
speculos/main.py 0.00% <0.00%> (ø)
speculos/mcu/apdu.py 0.00% <0.00%> (ø)
speculos/mcu/automation.py 91.48% <ø> (-1.24%) ⬇️
speculos/mcu/automation_server.py 0.00% <0.00%> (-40.43%) ⬇️
speculos/mcu/bagl.py 0.00% <0.00%> (ø)
speculos/mcu/button_tcp.py 0.00% <0.00%> (ø)
speculos/mcu/finger_tcp.py 0.00% <0.00%> (ø)
speculos/mcu/headless.py 0.00% <0.00%> (ø)
speculos/mcu/screen.py 0.00% <0.00%> (ø)
... and 13 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lpascal-ledger lpascal-ledger changed the title [Python module] Clearer code? [Python package] Clearer code? Jul 5, 2023
speculos/mcu/vnc.py Show resolved Hide resolved
…rface) and a Notifier (dispatching event from IOObjet or QtObject to the Display)
Copy link
Contributor

@fbeutin-ledger fbeutin-ledger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments especially at the top of classes would help a lot

speculos/mcu/vnc.py Outdated Show resolved Hide resolved
speculos/mcu/vnc.py Show resolved Hide resolved
speculos/mcu/vnc.py Outdated Show resolved Hide resolved
speculos/api/api.py Outdated Show resolved Hide resolved
speculos/mcu/apdu.py Outdated Show resolved Hide resolved
speculos/mcu/apdu.py Outdated Show resolved Hide resolved
speculos/mcu/bagl.py Show resolved Hide resolved
@lpascal-ledger lpascal-ledger merged commit 6a61aea into master Jul 20, 2023
36 checks passed
@lpascal-ledger lpascal-ledger deleted the lp/ci branch July 20, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants